Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tabbed Literate CoffeeScript #4345

Merged
merged 7 commits into from
Oct 23, 2016

Conversation

GeoffreyBooth
Copy link
Collaborator

I merged the #3924 pull request onto the 2 branch and updated it slightly, to fix #3924’s inability to work in browsers and to replace a magic string with a generated one that we check for uniqueness. This fixes #4336 (the bug introduced by #4313).

Per #3924, this uses the marked library to detect Markdown within .litcoffee files, to avoid false-positive markdown matches by the CoffeeScript compiler. The reason #3924 was never merged was because it introduced a non-development dependency, marked, which meant that the browser version of the CoffeeScript compiler was broken. I updated Cakefile to include node_modules/marked/lib/marked.js as part of the browser build, so now the browser compiler works with #3924. This immediately makes the browser version of CoffeeScript 20K larger, but I don’t think anyone’s using the browser version of CoffeeScript outside of testing environments like http://coffeescript.org/#try or http://js2.coffee/ so it probably doesn’t matter.

What do you all think? This at least fixes the broken test that plagues the 2 branch, is it worth merging into master as well?

@lydell
Copy link
Collaborator

lydell commented Oct 23, 2016

I think it's fine to merge this into 2, but I'd leave master the way it is.

(Kinda off-topic remark: In my opinion, there should be a 'literate-anything' package on npm that takes markdown as input and returns only the code of that markdown with a sourcemap. This literate thing hasn't got much to do with CoffeeScript, has it?)

@GeoffreyBooth
Copy link
Collaborator Author

Yes, good point. I think the pieces of such a package are already here in this PR: just use marked with the invertLiterate function, and there you go. A task for the reader? 😉

@GeoffreyBooth GeoffreyBooth merged commit 70a7265 into jashkenas:2 Oct 23, 2016
@lydell lydell deleted the fix-litcoffee branch October 23, 2016 17:08
@jashkenas
Copy link
Owner

It may fix the test, but including our first-ever dependency just for this is not so great. @lydell's approach would be nicer ;)

@GeoffreyBooth
Copy link
Collaborator Author

The dependency doesn't just fix the test, it also fixes bugs related to false positives in recognizing code versus comments in .litcoffee files. So it has some merit on its own.

But yes, maybe a better-still solution is to abstract out “literate anything” into a separate tool, and drop support for .litcoffee in the CoffeeScript compiler. Or the other way around, drop .litcoffee support and wait for someone else to build that tool or tools, if such a utility takes the form of a Gulp plugin, Webpack plugin etc.

@lydell
Copy link
Collaborator

lydell commented Oct 24, 2016

I'd vote for simply allowing dependencies in CoffeeScript 2.x.

@billymoon
Copy link
Contributor

@lydell I kinda came to the same conclusion about literate being non-coffee specific, and created https://github.com/billymoon/illiterate to test the idea out, take a look :)

@jashkenas
Copy link
Owner

I'd vote for simply allowing dependencies in CoffeeScript 2.x.

That's fine too.

@billymoon
Copy link
Contributor

@jashkenas I agree there is cost to introducing dependency, but in order to correctly implement literate feature, some kind of markdown parser is required, so either requiring dependency or including the code in repo. I expect even simple parser required to robustly parse markdown is sufficiently complex to be better off using externally maintained project.

In the end, I think the optimal approach would be to remove the literate feature from coffeescript altogether, and to create a general literate anything tool. Perhaps this tool could be supported in coffeescript more generally via a plugin system, or pre/post-processor system of some kind!? Shell pipeline works fine too of course.

@jashkenas
Copy link
Owner

In the end, I think the optimal approach would be to remove the literate feature from coffeescript altogether, and to create a general literate anything tool.

That would indeed be ideal. It could be a soft dependency, and CoffeeScript could try to use it when it encounters a .litcoffee or a .coffee.md file.

@GeoffreyBooth
Copy link
Collaborator Author

What is a “soft dependency,” and how could CoffeeScript use it conditionally? Isn’t a dependency either in package.json or not in it?

And what’s the harm in having dependencies, in 1.x or 2? Aside from growing the browser version of the compiler, which no one should be using in production anyway. (Though one could argue that there’s no need for the browser version of CoffeeScript to know how to parse Literate CoffeeScript.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants